CI | seperate noobaa image build to different action #9051
Conversation
nadavMiz
force-pushed
the
ci-build-noobaa
branch
2 times, most recently
from
ddf51c5 to
2e5bdf1
Compare
nadavMiz
force-pushed
the
ci-build-noobaa
branch
23 times, most recently
from
9387673 to
2a39817
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ceph-nsfs-s3-tests.yaml (1)
16-23: Validate artifact consumption steps
Ensure the artifact namenoobaa-testerand path/tmpmatch the build workflow output, and verifydocker loaduses the correct file.
🧹 Nitpick comments (2)
.github/workflows/unit.yaml (1)
19-19: Remove trailing whitespace
There are trailing spaces on this line; please remove them to satisfy YAML linting.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: trailing spaces
(trailing-spaces)
.github/workflows/ceph-nsfs-s3-tests.yaml (1)
31-31: Add newline at end of file
Please ensure there is a trailing newline character to satisfy YAML formatting conventions.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 31-31: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/warp-tests.yaml
- .github/workflows/nc_unit.yml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/sanity-ssl.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ceph-nsfs-s3-tests.yaml
[error] 31-31: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/unit.yaml
[error] 19-19: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-noobaa-image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (8)
.github/workflows/unit.yaml (4)
2-2: Switch to reusable workflow trigger
Usingworkflow_callis correct to make this a reusable workflow.
6-6: Explicit job naming
Therun-unit-testsjob name is clear and aligns with its purpose.
9-9: Set job permissions appropriately
Usingpermissions: read-allis necessary for artifact download.
14-21: Validate artifact download and image load
Confirm that thenoobaa-testerartifact tarball is correctly downloaded to/tmp/noobaa-tester.tarand thatdocker loadpoints to the correct location.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: trailing spaces
(trailing-spaces)
.github/workflows/ceph-nsfs-s3-tests.yaml (4)
2-2: Switch to reusable workflow trigger
Usingworkflow_callensures this workflow can be invoked from the orchestrator.
8-8: Set job permissions appropriately
Grantingread-allis required for artifact download and image loading.
10-14: Simplify checkout step
The updated checkout step correctly specifies therepositoryand clean path without unnecessary quotes.
31-31: Include tester option in test command
The-o testerflag correctly instructs the test suite to use the downloaded tester image.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 31-31: no new line character at the end of file
(new-line-at-end-of-file)
nadavMiz
force-pushed
the
ci-build-noobaa
branch
from
51c8f90 to
4213fef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/unit.yaml (2)
19-19: Remove trailing whitespace.
Line 19 contains unnecessary spaces – trimming them improves YAML lint.- path: /tmp + path: /tmp🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: trailing spaces
(trailing-spaces)
27-27: Eliminate extra blank line.
YAMLLint flags too many blank lines; remove the empty line at the end for consistency.-🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/ceph-s3-tests.yaml
- .github/workflows/nc_unit.yml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/warp-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
- GitHub Check: build-noobaa-image
🔇 Additional comments (6)
.github/workflows/unit.yaml (6)
2-2: Confirmed trigger update toworkflow_call.
Switching frompush/pull_requesttoworkflow_callis correct for a reusable workflow.
6-6: Job name made explicit.
Renaming the job torun-unit-testsimproves readability in the workflow UI.
9-9: Permissions set toread-all.
This aligns with other workflows and ensures artifact download and checkout permissions.
14-18: Artifact download step added.
Pulling thenoobaa-testerimage here matches the new orchestrator workflow. Ensure the artifact name and path (/tmp) align with the build step in yourrun-pr-tests.yaml.
21-21: Load Docker image from artifact.
docker load --input /tmp/noobaa-tester.tarcorrectly restores the tester image.
25-26: Test commands updated to use tester image.
Adding-o testerto bothmake testandmake root-perm-testensures the prebuilt image is used.
nadavMiz
force-pushed
the
ci-build-noobaa
branch
2 times, most recently
from
027a13f to
de82d75
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.github/workflows/unit.yaml (5)
2-2: Prefer mapping syntax for triggers
Whileon: [workflow_call]is valid shorthand, consider using the mapping style for clarity and to easily add inputs later:-on: [workflow_call] +on: + workflow_call:
6-6: Jobnameduplicates its ID; consider human-friendly naming
Thenamefield mirrors the job IDrun-unit-tests. You can omit it or use a more descriptive title, e.g.:-name: run-unit-tests +name: Run Unit Tests
14-18: Use runner-provided temp directory instead of hard-coded/tmp
Replace/tmpwith${{ runner.temp }}for portability and to follow best practices:- - name: Download artifact - uses: actions/download-artifact@v4 - with: - name: noobaa-tester - path: /tmp + - name: Download artifact + uses: actions/download-artifact@v4 + with: + name: noobaa-tester + path: ${{ runner.temp }}
19-19: Remove trailing whitespace
Line 19 contains trailing spaces—trim them to satisfy linting rules:🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: trailing spaces
(trailing-spaces)
27-27: Remove extra blank line
The extra blank line at the end of the file is flagged by linting—remove it to adhere to file formatting guidelines.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
🛑 Comments failed to post (1)
.github/workflows/unit.yaml (1)
9-9: 🛠️ Refactor suggestion
Restrict permissions to principle of least privilege
Usingread-allgrants broad access. Audit the actual scopes needed (e.g.,contents: read,packages: read) and narrow this down to minimize risk.🤖 Prompt for AI Agents
In .github/workflows/unit.yaml at line 9, the permissions setting uses 'read-all', which grants overly broad access. Review the workflow's actual needs and replace 'read-all' with the minimal required scopes such as 'contents: read' or 'packages: read' to adhere to the principle of least privilege and reduce security risks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/unit.yaml (4)
2-2: Declare workflow inputs for clarity.Since this is a reusable workflow triggered by
workflow_call, consider defininginputs:underon: workflow_call(e.g., artifact name, tester tag) to make the expected parameters explicit.
9-9: Scope down permissions.Avoid
permissions: read-all; specify only needed scopes (e.g.,actions: read,contents: read) to adhere to least privilege.
20-21: Validate Docker load input.Consider adding a preliminary check (e.g.,
ls /tmp/noobaa-tester.tar) to fail fast with a clear message if the tarball isn't present before runningdocker load.
19-19: Remove trailing spaces & blank lines.Clean up these lines to address YAML lint warnings (
trailing-spaces,empty-lines).Also applies to: 27-27
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/nc_unit.yml
- .github/workflows/warp-tests.yaml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/sanity.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-jest-unit-tests
- GitHub Check: build-noobaa-image
🔇 Additional comments (2)
.github/workflows/unit.yaml (2)
14-18: Verify artifact name and structure.Ensure the upstream
build-noobaajob uploads an artifact namednoobaa-testercontaining the.tarat the root so thatactions/download-artifactcorrectly restores/tmp/noobaa-tester.tar.
25-26: Test commands updated correctly.Adding
-o testermatches the new image loading logic and appears correct.
nadavMiz
force-pushed
the
ci-build-noobaa
branch
from
de82d75 to
25e0998
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/unit.yaml (3)
14-18: Download and prepare the NooBaa tester artifact.Downloading the
noobaa-testerartifact and placing it in/tmpallows subsequent steps to load the Docker image. Confirm the artifact name matches the producer workflow and consider using the runner’s temp directory ($RUNNER_TEMP) instead of hardcoding/tmpfor better portability.Would you like a sample update using
$RUNNER_TEMPfor artifact download path?
19-19: Remove trailing spaces.Static analysis flagged trailing whitespace on this line; please remove it to avoid lint errors.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: trailing spaces
(trailing-spaces)
27-27: Remove extra blank line.Static analysis warns of an unnecessary blank line here; removing it will keep the file tidy.
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/nc_unit.yml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/sanity.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-jest-unit-tests
- GitHub Check: build-noobaa-image
🔇 Additional comments (4)
.github/workflows/unit.yaml (4)
2-2: Enable reusable workflow trigger.Switching to
on: [workflow_call]makes this workflow reusable only viaworkflow_call, which aligns with the centralization objective. Ensure no unintended triggers are needed.
6-6: Explicit job naming improves clarity.The job is now explicitly named
run-unit-tests, making outputs more readable and logs easier to locate.
20-21: Load the Docker image.The
docker loadcommand correctly imports the tester image for use in this job. Consider adding anidto this step if later steps need to reference its output.
25-26: Include tester option in test commands.Appending
-o testerto bothmake testandmake root-perm-testensures the correct image is used. Confirm that both commands exit on failure (set -eis default), so the step fails appropriately if tests fail.
nadavMiz
force-pushed
the
ci-build-noobaa
branch
from
25e0998 to
ccefb3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/unit.yaml (3)
14-18: Consider using GitHub Container Registry instead of tar artifacts
Pulling and extracting a tarball adds latency. As noted in the PR objectives, you could push these images to GHCR anddocker pullthem directly in your workflows to speed up CI and reduce storage overhead.
19-19: Remove trailing whitespace
Line 19 contains only spaces. Please delete trailing spaces to satisfy YAML lint rules and avoid churn in diffs.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: trailing spaces
(trailing-spaces)
27-27: Eliminate extra blank line
YAML lint warns about too many consecutive blank lines. Please remove the redundant empty line at 27.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/sanity.yaml
- .github/workflows/nc_unit.yml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[warning] 9-9: too many spaces after colon
(colons)
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 27-27: too many blank lines
(1 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-noobaa-image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (3)
.github/workflows/unit.yaml (3)
2-2: Verifyworkflow_call-only trigger
You’ve removedpush/pull_requesttriggers in favor ofon: [workflow_call]. Ensure this workflow is always invoked by your orchestrator (run-pr-tests.yaml) or add alternative triggers (e.g.,workflow_dispatch) if you still need to run tests directly.
20-21: Loading Docker image looks correct
Thedocker load --input /tmp/noobaa-tester.tarstep properly restores the tester image for downstream steps.
25-26: Updated test commands are good
Including-o testeron bothmake testandmake root-perm-testaligns with the new image-loading approach.
nadavMiz
force-pushed
the
ci-build-noobaa
branch
from
ccefb3a to
d9666c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/unit.yaml (3)
21-22: Consider using container registry instead of TAR artifacts
Loading images from tar introduces overhead. You could explore using GitHub Container Registry (GHCR) to push/pull Docker images directly and speed up CI.
20-20: Remove trailing spaces
Line 20 contains trailing spaces; remove them to satisfy YAML lint rules.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 20-20: trailing spaces
(trailing-spaces)
28-28: Remove extra blank line
There's an extra blank line at the end of the multi-line script. Removing it will satisfy the empty-lines lint rule.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 28-28: too many blank lines
(1 > 0) (empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/nc_unit.yml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/sanity.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[error] 20-20: trailing spaces
(trailing-spaces)
[warning] 28-28: too many blank lines
(1 > 0) (empty-lines)
🔇 Additional comments (4)
.github/workflows/unit.yaml (4)
2-2: Ensure correct invocation with workflow_call
Switching toworkflow_callmeans this workflow no longer runs on push/PR. Verify that the orchestrator workflow (run-pr-tests.yaml) correctly calls this action with the required inputs.
8-10: Principle of least privilege applied to permissions
Grantingactions: readandcontents: readscopes down access appropriately and satisfies checkout and artifact download requirements.
15-19: Artifact download step is correctly configured
Theactions/download-artifactstep pulls thenoobaa-testerimage tar from/tmp. Ensure the artifact name matches exactly what the build workflow produces.
26-27: Test commands updated with-o tester
Including the-o testerflag aligns with the loaded tester image. Confirm that all downstream test workflows have similar updates for consistency.
nadavMiz
force-pushed
the
ci-build-noobaa
branch
from
d9666c0 to
1346009
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/unit.yaml (2)
15-19: Validate artifact naming and consider a registry-based approach.
Ensure the artifact namenoobaa-testerexactly matches what the build-noobaa workflow publishes to avoid download failures.
As an optimization, consider pushing the Docker image to GitHub Container Registry (ghcr.io/.../noobaa-tester:latest) and usingdocker pullhere to eliminate tar export/import overhead.
20-20: Remove trailing whitespace and extra blank lines.
Clean up the blank lines and trailing spaces to satisfy YAML lint rules.- (line 20) <trailing spaces> - (line 23) - (line 28) +<remove these blank lines>Also applies to: 23-23, 28-28
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 20-20: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/warp-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/nc_unit.yml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
- .github/workflows/sanity-ssl.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[error] 20-20: trailing spaces
(trailing-spaces)
[warning] 28-28: too many blank lines
(1 > 0) (empty-lines)
🔇 Additional comments (4)
.github/workflows/unit.yaml (4)
2-2: Workflow trigger updated toworkflow_call.
This aligns with the reusable workflow pattern and prevents redundant CI runs on direct pushes/pull requests.
8-10: Permissions scoped to least privilege.
Grantingactions: readandcontents: readprovides only the necessary access for artifact download and checkout.
21-22: Docker image loading step is correct.
Thedocker load --input /tmp/noobaa-tester.tarcommand properly restores the tester image for subsequent test commands.
26-27: Test commands updated for preloaded image.
Including-o testerensures tests run against the downloaded tester image as intended.
nadavMiz
force-pushed
the
ci-build-noobaa
branch
2 times, most recently
from
a6d4e81 to
86636a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/unit.yaml (2)
20-20: Remove trailing whitespace
Line 20 contains trailing spaces, which may trigger lint warnings. Consider deleting the extra spaces.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 20-20: trailing spaces
(trailing-spaces)
28-28: Remove extra blank line
Avoid excessive blank lines to adhere to YAMLlint's empty-lines rule. Consider deleting this line.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 28-28: too many blank lines
(1 > 0) (empty-lines)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/nc_unit.yml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/sanity.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[error] 20-20: trailing spaces
(trailing-spaces)
[warning] 28-28: too many blank lines
(1 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-noobaa-image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (5)
.github/workflows/unit.yaml (5)
2-2: Workflow trigger switched toworkflow_call
This aligns the workflow for reuse in composite actions.
8-10: Permissions scoped for artifact download and checkout
Grantingactions: readandcontents: readprovides least privilege needed for downloading artifacts and checking out code.
15-19: Added step to download the NooBaa tester image artifact
Usingactions/download-artifactto fetch thenoobaa-testertarball is correct for loading the Docker image later.
21-23: Load the tester Docker image
Thedocker loadcommand correctly imports the image for subsequent test steps.
26-27: Include-o testeroption in test commands
Passing thetesterimage to bothmake testandmake root-perm-testensures tests run inside the correct service context.
Signed-off-by: nadav mizrahi <nadav.mizrahi16@gmail.com>
nadavMiz
force-pushed
the
ci-build-noobaa
branch
from
86636a3 to
11dc341
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/unit.yaml (3)
15-19: Use runner-provided temp directory for downloads
Hardcoding/tmpmay not be portable on non-Linux runners. Prefer the${{ runner.temp }}environment variable for temporary storage:- with: - name: noobaa-tester - path: /tmp + with: + name: noobaa-tester + path: ${{ runner.temp }}
21-23: Consider image cleanup or registry caching
Repeated tar loads can consume runner disk and time. You could:
- Pull the image from GitHub Container Registry instead of artifacts.
- Add a cleanup step before or after to remove stale images (
docker rmi noobaa-tester).
20-20: Remove trailing whitespace and extra blank lines
YAMLlint flagged trailing spaces at line 20 and an extra blank line at line 28. Please remove them to satisfy lint rules.Also applies to: 28-28
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 20-20: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ceph-nsfs-s3-tests.yaml(1 hunks).github/workflows/ceph-s3-tests.yaml(1 hunks).github/workflows/nc_unit.yml(1 hunks).github/workflows/postgres-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(1 hunks).github/workflows/sanity-ssl.yaml(1 hunks).github/workflows/sanity.yaml(1 hunks).github/workflows/unit.yaml(1 hunks).github/workflows/warp-nc-tests.yaml(2 hunks).github/workflows/warp-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/nc_unit.yml
- .github/workflows/postgres-unit-tests.yaml
- .github/workflows/warp-tests.yaml
- .github/workflows/sanity.yaml
- .github/workflows/sanity-ssl.yaml
- .github/workflows/run-pr-tests.yaml
- .github/workflows/ceph-nsfs-s3-tests.yaml
- .github/workflows/warp-nc-tests.yaml
- .github/workflows/ceph-s3-tests.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/unit.yaml
[error] 20-20: trailing spaces
(trailing-spaces)
[warning] 28-28: too many blank lines
(1 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run-package-lock-validation
- GitHub Check: build-noobaa-image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (3)
.github/workflows/unit.yaml (3)
2-2: Switch to reusable trigger via workflow_call
Changing triggers toworkflow_callaligns the workflow with the new orchestrator pattern. Ensure downstream workflows pass all required inputs when invoking this.
8-10: Least-privilege permissions scoped correctly
Grantingactions: readandcontents: readis sufficient for artifact download and checkout. No broadread-allscope—good adherence to the principle of least privilege.
26-27: Test commands updated for tester image
Appending-o testeraligns with loading the tester image. Verify that all relevantmaketargets support this flag.
3 participants
Describe the Problem
currently noobaa and tester image are built for each task separately. causing extra load on the CI resources
Explain the Changes
###Gaps
Testing Instructions:
Summary by CodeRabbit
New Features
Chores